Proper EDNS handling and cleaner NOERROR response in DNSSERVER #11411
Conversation
|
|
👋 Hello Kolkman, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
| @@ -1,5 +1,5 @@ | |||
| name=DNSServer | |||
| version=3.2.0 | |||
| version=3.3.0 | |||
There was a problem hiding this comment.
this version is updated with every new release. Please revert
There was a problem hiding this comment.
Reverted the change with new commit
| @@ -9,6 +9,23 @@ | |||
| #define DNS_OFFSET_DOMAIN_NAME DNS_HEADER_SIZE // Offset in bytes to reach the domain name labels in the DNS message | |||
| #define DNS_DEFAULT_PORT 53 | |||
|
|
|||
| #define DNS_SOA_MNAME_LABEL "ns" | |||
| #define DNS_SOA_RNAME_LABEL "esp32" | |||
There was a problem hiding this comment.
what are MNAME and RNAME? How are their values chosen?
There was a problem hiding this comment.
MNAME and RNAME are values defined in RFC1035 section 3.1.13.
Tne MNAME is the name of the primary nameserver the request came from - that value is only used in the context of dynamic DNS updates RFC 2136. In this contexts it is purely informational. I chose the 'ns' label as that is sort of customary for nameservers.
The RNAME is used to construct the email address of the administrator for the zone. The way things are coded now that is esp32@local. There nothing in the DNS protocol that uses that value, it is only for network administrator information siganling purpose.
reverting version number update - as it is done automatically
|
@Kolkman small typo reported by the CI: https://github.com/espressif/arduino-esp32/actions/runs/15443489960/job/43466846177?pr=11411#step:9:29 |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the DNSServer to properly ignore EDNS OPT records and return an RFC-compliant "No Data" (NODATA) response with an SOA record when the queried name exists but the type does not match.
- Ignore the additional section (EDNS OPT) by zeroing
ARCountso queries with OPT still match. - Branch
_handleUDPto callreplyWithNoAnswfor non-A/ANY queries, implementing NODATA per RFC2308. - Introduce SOA constants and implement
replyWithNoAnswto emit an authority SOA RR with minimal TTL.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| libraries/DNSServer/src/DNSServer.h | Added SOA‐related macros and declared replyWithNoAnsw method. |
| libraries/DNSServer/src/DNSServer.cpp | Updated _handleUDP logic, ignore EDNS in requestIncludesOnlyQuestion, implemented replyWithNoAnsw. |
Comments suppressed due to low confidence (2)
libraries/DNSServer/src/DNSServer.h:199
- [nitpick] The method name
replyWithNoAnswis abbreviated and may be unclear; consider renaming toreplyWithNoAnswerorreplyWithNoDatafor clarity.
inline void replyWithNoAnsw(AsyncUDPPacket &req, DNSHeader &dnsHeader, DNSQuestion &dnsQuestion);
libraries/DNSServer/src/DNSServer.cpp:128
- [nitpick] Add a unit or integration test to verify that queries with an OPT record (non-zero ARCount) are correctly ignored and still receive a response.
dnsHeader.ARCount = 0; // We assume that if ARCount !=0 there is a EDNS OPT packet, just ignore
Test Results 76 files 76 suites 12m 44s ⏱️ Results for commit 557d9c7. ♻️ This comment has been updated with latest results. |
Description of Change
Bugs
The code solves two bugs:
BUG 1: When trouble shooting a dnsserver with the 'dig' (one of the main DNS troubleshooting tools) the server doesn't answer because it doesn't handle EDNS (OPT records in the additional section).
That has been fixed, the additional section in the query can safely be ignored.
(mostly change around line 123)
BUG 2: The code also returns an A record when the QUERY Type not 'A' (or 'ANY').
This has been fixed too. For an A and ANY query type the server returns an A record.
(mostly change around line 114)
Feature
The code continues to return a 'SERVFAIL' when no match with QNAME occurs. However, with the fix of bug 2 we created a proper 'No Data' response, (RFC2308 2.2) for queries that have a match for the name, but not for the type. The function DNSServer::replyWithNoAnsw implements that functionality.
The server now behaves somewhat more conform the DNS protocl specifications.
(most of the code changes)
Tests scenarios
I tested this code on an ESP32 and WireShark as protocol analyzer - there is no reason to suspect that it does not work on other platforms (to which I have no access).
Related links